lkl: remove string functions duplicate implementation#590
Conversation
ddiss
left a comment
ddiss
left a comment
There was a problem hiding this comment.
Looks like a nice cleanup. One concern: lkl_ops is set at run-time, so we could have a "posix" host call lkl_init() with e.g. lkl_host_operations.memset = NULL.
It looks as though the main reason for using run-time ops instead of build-time is for overloading the print() hook, so perhaps we could switch the API to use build-time ops and provide print() hooks elsewhere? Another (simpler) option would be to fail lkl_init() if any of the mem ops are NULL alongside CONFIG_LKL_HOST_MEM*=y.
Good catch!
The original intent was to have LKL distributed as a shared library and allow external applications to override run-time ops. In practice, because we lack support for modules the former is not yet achievable. On one hand having runtime host ops gives more flexibility to applications, on the other hand built-time host ops would probably give a nice performance boost.
Yes, this is a good option while we discuss more built-in vs run-time host ops. I'll send another version to add these checks. |
| @@ -6,6 +6,25 @@ int lkl_init(struct lkl_host_operations *ops) | |||
| { | |||
| lkl_ops = ops; | |||
There was a problem hiding this comment.
Please put this after the config checks. I don't think lkl_ops should be initialized on failure...
(edit: kasan_init() failure should probably leave it unset too, but that can be done separately)
| return -1; | ||
| } | ||
| #endif | ||
|
|
There was a problem hiding this comment.
coding-style.rst encourages avoiding #ifdef CONFIG_... outside of header files. How about something like:
if ((IS_ENABLED(CONFIG_LKL_HOST_MEMCPY) && !ops->memcpy)
|| (IS_ENABLED(CONFIG_LKL_HOST_MEMSET) && !ops->memset)
|| (IS_ENABLED(CONFIG_LKL_HOST_MEMMOVE) && !ops->memmove)) {
lkl_printf("unexpected NULL lkl_host_ops memory I/O call\n");
return -1;
}
The compiler should still be able to optimize it out for non-posix hosts.
tavip
force-pushed
the
lkl-rm-str-dup
branch
2 times, most recently
from
db68edc to
f4dbc3c
Compare
|
|
||
| int lkl_init(struct lkl_host_operations *ops) | ||
| { | ||
| int ret; |
Now that we have the ability to set kernel config options per host built we can avoid duplicating the implementation for string functions that may be provided by the host (e.g. memcpy, memset). Signed-off-by: Octavian Purdila <tavip@google.com>
Check lkl_init for failures. Print the log buffer in case of failures, to make debugging easier. Signed-off-by: Octavian Purdila <tavip@google.com>
4 participants
Now that we have the ability to set kernel config options per host built we can avoid duplicating the implementation for string functions that may be provided by the host (e.g. memcpy, memset).
Fixes: #582